Ensure minimum RBF feerate satisfies BIP125#4494
Ensure minimum RBF feerate satisfies BIP125#4494jkczyz wants to merge 2 commits intolightningdevkit:mainfrom
Conversation
|
👋 Thanks for assigning @wpaulino as a reviewer! |
72455fc to
1098cc3
Compare
The spec's 25/24 multiplier doesn't always satisfy BIP125's relay requirement of an absolute fee increase. Use a flat +25 sat/kwu (0.1 sat/vB) increment for our own RBFs instead, while still accepting the 25/24 rule from counterparties. Extract a `min_rbf_feerate` helper to consolidate the two call sites and add a test that a counterparty feerate satisfying 25/24 (but not +25) is accepted. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1098cc3 to
9b9c114
Compare
|
Will need for proper integration testing of LDK Node's |
| /// Returns the minimum feerate for our own RBF attempts given a previous feerate. | ||
| /// | ||
| /// The spec (tx_init_rbf) requires the new feerate to be >= 25/24 of the previous feerate. | ||
| /// However, that multiplier doesn't always satisfy BIP125's relay requirement of an absolute fee | ||
| /// increase, so for our own RBFs we use a flat +25 sat/kwu (0.1 sat/vB) increment instead. We | ||
| /// still accept the 25/24 rule from counterparties in [`FundedChannel::validate_tx_init_rbf`]. | ||
| fn min_rbf_feerate(prev_feerate: u32) -> FeeRate { | ||
| FeeRate::from_sat_per_kwu((prev_feerate as u64).saturating_add(25)) | ||
| } |
There was a problem hiding this comment.
Bug: For prev_feerate > 600, this returns a value below the spec's 25/24 rule that counterparties enforce in validate_tx_init_rbf (line 12719). This means our own RBF attempts will be rejected by any spec-compliant counterparty at higher feerates.
Example at prev_feerate = 1000:
min_rbf_feeratereturns 1025- Counterparty checks
1025 * 24 = 24600 < 1000 * 25 = 25000→ rejected
The crossover is at exactly 600 sat/kwu (25 * 24 = 600). Above that, +25 is insufficient to satisfy >= ceil(prev * 25/24). Real-world feerates are often well above 600 sat/kwu.
Additionally, maybe_adjust_for_rbf (line 12172) adjusts contributions to this floor, so automated feerate adjustment will also produce rejected RBFs.
Consider using max(prev + 25, ceil(prev * 25/24)) to satisfy both the BIP125-motivated flat increment and the spec's multiplicative rule:
| /// Returns the minimum feerate for our own RBF attempts given a previous feerate. | |
| /// | |
| /// The spec (tx_init_rbf) requires the new feerate to be >= 25/24 of the previous feerate. | |
| /// However, that multiplier doesn't always satisfy BIP125's relay requirement of an absolute fee | |
| /// increase, so for our own RBFs we use a flat +25 sat/kwu (0.1 sat/vB) increment instead. We | |
| /// still accept the 25/24 rule from counterparties in [`FundedChannel::validate_tx_init_rbf`]. | |
| fn min_rbf_feerate(prev_feerate: u32) -> FeeRate { | |
| FeeRate::from_sat_per_kwu((prev_feerate as u64).saturating_add(25)) | |
| } | |
| fn min_rbf_feerate(prev_feerate: u32) -> FeeRate { | |
| let flat_increment = (prev_feerate as u64).saturating_add(25); | |
| let spec_increment = ((prev_feerate as u64) * 25).div_ceil(24); | |
| FeeRate::from_sat_per_kwu(cmp::max(flat_increment, spec_increment)) | |
| } |
There was a problem hiding this comment.
Using max now which also matches the alternative spec proposal.
|
No issues found. All three issues from my prior review have been addressed in this revision:
The new test |
a18dac2 to
a8d3bda
Compare
lightning/src/ln/splicing_tests.rs
Outdated
| // Node 1 initiates a splice at a HIGH feerate (10,000 sat/kwu). The minimum RBF feerate will be | ||
| // 25/24 of 10,000 = 10,417 sat/kwu — far above what node 0's tight budget can handle. | ||
| // 10,000 + 25 = 10,025 sat/kwu — far above what node 0's tight budget can handle. |
There was a problem hiding this comment.
Nit: This comment is incorrect. At 10,000 sat/kwu, the 25/24 rule dominates over +25:
min_rbf_feerate(10000)=max(10025, ceil(10000 * 25/24))=max(10025, 10417)= 10417
The comment was mechanically updated from "25/24 of 10,000 = 10,417" to "+25 = 10,025" but the crossover point is 600 sat/kwu, and 10,000 is well above it.
| // Node 1 initiates a splice at a HIGH feerate (10,000 sat/kwu). The minimum RBF feerate will be | |
| // 25/24 of 10,000 = 10,417 sat/kwu — far above what node 0's tight budget can handle. | |
| // 10,000 + 25 = 10,025 sat/kwu — far above what node 0's tight budget can handle. | |
| // Node 1 initiates a splice at a HIGH feerate (10,000 sat/kwu). The minimum RBF feerate will be | |
| // max(10,000 + 25, ceil(10,000 * 25/24)) = 10,417 sat/kwu — far above what node 0's tight budget can handle. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4494 +/- ##
==========================================
- Coverage 86.24% 86.20% -0.04%
==========================================
Files 160 161 +1
Lines 107909 108611 +702
Branches 107909 108611 +702
==========================================
+ Hits 93061 93625 +564
- Misses 12212 12351 +139
+ Partials 2636 2635 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
The flat +25 sat/kwu increment falls below the spec's 25/24 multiplicative rule for feerates above 600 sat/kwu, which would cause our RBF attempts to be rejected by spec-compliant counterparties. Take the max of both to satisfy BIP125 relay requirements at low feerates and the spec rule at higher feerates. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
a8d3bda to
f56d3c8
Compare
The spec's 25/24 multiplier doesn't always satisfy BIP125's relay requirement of an absolute fee increase at low feerates, while a flat +25 sat/kwu increment falls below the spec's 25/24 rule above 600 sat/kwu. Use
max(prev + 25, ceil(prev * 25/24))for our own RBFs to satisfy both constraints, while still accepting the bare 25/24 rule from counterparties.Potential spec change in lightning/bolts#1327.